Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add optional file flags for cli view add command #3314

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

feat: add optional file flags for cli view add command #3314

wants to merge 4 commits into from
+60 −25

Conversation

najeal
Copy link

@najeal najeal commented Dec 11, 2024 •
edited
Loading

Relevant issue(s)

Resolves #3280

Description

This PR adds add optional way to provide query and sdl data from a file when using cli view add command.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

It has been tested adding new unit tests.

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle
Copy link
Collaborator

Hi @najeal. Thank you for contributing this PR.

The pattern in your implementation is clever but makes it hard for users to understand easily how to use the command as mixes ordered args and unordered flags.

I would recommend switching entirely to flags like we do with some of our other commands. Cobra has some functionality to do an exclusif or on a set of flags so we could easily do.

--query xor --query-file
--sdl xor --sdl-file
--lens xor --lens-file

Once/if you decide to do the change, please update the command usage description accordingly :)

@najeal
Copy link
Author

najeal commented Dec 12, 2024 •
edited
Loading

Hi @fredcarle ! I will do the changes 👍

@najeal
Copy link
Author

najeal commented Dec 12, 2024

Changes are done 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add file options to View CLI add command
2 participants
@najeal @fredcarle